- 
                Notifications
    
You must be signed in to change notification settings  - Fork 51
 
Allow selenium image configuration #268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| 
           I like to be able to specify the images... just guessing when we should follow the  Right now it's really a mix of env and options, grrr. Need to think a little bit more about it...  | 
    
| 
           OT: @NoelDeMartin, I've force-pushed (without changes) your candidate branch to get the tests executed, it seems that some of them were stuck.  | 
    
          Codecov ReportAll modified and coverable lines are covered by tests ✅ 
 Additional details and impacted files@@             Coverage Diff              @@
##               main     #268      +/-   ##
============================================
+ Coverage     85.42%   87.89%   +2.46%     
- Complexity      722      725       +3     
============================================
  Files            75       75              
  Lines          2216     2222       +6     
============================================
+ Hits           1893     1953      +60     
+ Misses          323      269      -54     ☔ View full report in Codecov by Sentry.  | 
    
d8c573c    to
    15ee698      
    Compare
  
    | 
           Thanks @stronk7, I've implemented the  It seems like some tests are failing in Travis but I cannot see the details.  | 
    
          
 Yeah, Travis is being a nightmare since 2 weeks ago. Sometimes it installs PostgreSQL on port 5432, others in 5432... I've been fighting with that in #270 , just to, at the end, leave the port like it's now (5432). Will look to this soon... thanks!  | 
    
15ee698    to
    97f2ba7      
    Compare
  
    | 
           Side note, surely this closes #15  | 
    
| 
           Thanks @NoelDeMartin, it's 98% perfect for me! The remaining 2% is: 
 Thoughts?  | 
    
78f8cc3    to
    7585e2c      
    Compare
  
    | 
           Thanks for the review @stronk7, I have applied your suggestions: 
  | 
    
          
 Yeah, I was looking for it in the Docs and only the "quick start" section of https://moodlehq.github.io/moodle-plugin-ci/Help.html was showing some options. I've created #271 about that, surely we'll need to make a few new pages (similar to the app one) and document many of those environmental variables. In any case that falls out of this PR scope. Ciao :-)  | 
    
| 
           I will merge this as soon as we are green (tests still running). Thanks for your work and patience!  | 
    
7585e2c    to
    0624fc0      
    Compare
  
    | 
           Sold!  | 
    
I've decided to remove the private properties because they were only used once, and now everything is encapsulated in the
getSeleniumImagemethod.Other than that, there isn't much more to the PR. It allows to configure the selenium image by setting
MOODLE_BEHAT_SELENIUM_IMAGE, or specify it for each profile withMOODLE_BEHAT_SELENIUM_CHROME_IMAGEandMOODLE_BEHAT_SELENIUM_FIREFOX_IMAGE.I also pondered whether to allow indicating only the browser version. For example, setting
MOODLE_BEHAT_CHROME_VERSIONor acceptingchrome:117for the profile. But I wasn't convinced by that approach, so I ended up using this one that also allows customizing the image completely (for example, in case you'd need to use seleniarm or something else).